Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Complex forms in miniLcm #1061

Open
wants to merge 23 commits into
base: develop
Choose a base branch
from
Open

Complex forms in miniLcm #1061

wants to merge 23 commits into from

Conversation

hahn-kev
Copy link
Collaborator

@hahn-kev hahn-kev commented Sep 23, 2024

this is just a first pass at what Complex Forms might look like. I'd like to get some input on how well this models complex forms and what might be missing

  • reading
  • writing changes (backend only)
  • CRDT

Copy link

github-actions bot commented Sep 23, 2024

C# Unit Tests

75 tests  ±0   75 ✅ ±0   6s ⏱️ -1s
13 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 2652efc. ± Comparison against base commit 4a029e7.

♻️ This comment has been updated with latest results.

@hahn-kev hahn-kev changed the title Complex forms ini miniLcm Complex forms in miniLcm Sep 24, 2024
@hahn-kev hahn-kev mentioned this pull request Sep 27, 2024
2 tasks
@hahn-kev hahn-kev requested a review from myieye October 9, 2024 05:36
Copy link
Contributor

@myieye myieye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got a few minor suggestions and such. But, I don't see anything that's really wrong here.

}

[Fact]
public async Task CreateEntry_WithComponentSenseDoesNotShowOnComplexFormsList()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public async Task CreateEntry_WithComponentSenseDoesNotShowOnComplexFormsList()
public async Task CreateEntry_WithComponentSenseDoesNotShowOnEntryComplexFormsList()

@@ -15,22 +16,25 @@ namespace FwDataMiniLcmBridge.Api;

public class FwDataMiniLcmApi(Lazy<LcmCache> cacheLazy, bool onCloseSave, ILogger<FwDataMiniLcmApi> logger, FwDataProject project) : IMiniLcmApi, IDisposable
{
private LcmCache Cache => cacheLazy.Value;
public LcmCache Cache => cacheLazy.Value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would InternalsVisibleTo be more appropriate?

Senses = entry.AllSenses.Select(FromLexSense).ToList(),
ComplexFormTypes = ToComplexFormType(entry),
Components = ToComplexFormComponents(entry),
//todo, this does not include complex forms which reference a sense
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember what you said about this TODO. Is it tricky? Can we not just put this on Sense:
ComplexForms = [..sense.ComplexFormEntries.Select(complexEntry => ToSenseReference(sense, complexEntry))]

};
}

private IList<ComplexFormType> ToComplexFormType(ILexEntry entry)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private IList<ComplexFormType> ToComplexFormType(ILexEntry entry)
private IList<ComplexFormType> ToComplexFormTypes(ILexEntry entry)

crdtEntries.Should().BeEquivalentTo(fwdataEntries,
options => options
.For(e => e.Components).Exclude(c => c.Id)
.For(e => e.Components).Exclude(c => c.ComponentHeadword)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is due to the fact that we don't yet ever change the headword on components or complex forms in CRDTs?

{
entity.ComplexFormTypes =
[
..entity.ComplexFormTypes.Where(t => t.Id != OldComplexFormTypeId),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current plan is to eventually use a sort-order field for persisting sorting, right?

{
if (ComplexFormEntryId.HasValue)
{
entity.ComplexFormEntryId = ComplexFormEntryId.Value;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to update headwords here? We could presumably pull them out of the context.
Or are you just ignoring them, because we don't know how to update them when the headword of a referenced entry changes?

Comment on lines +219 to +221
//could optimize this by doing a single query, but this is easier to read
entry.Components = [..await ComplexFormComponents.Where(c => c.ComplexFormEntryId == id).ToListAsyncEF()];
entry.ComplexForms = [..await ComplexFormComponents.Where(c => c.ComponentEntryId == id).ToListAsyncEF()];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a pretty small legibility sacrifice to make for a whole db query 🤷

Suggested change
//could optimize this by doing a single query, but this is easier to read
entry.Components = [..await ComplexFormComponents.Where(c => c.ComplexFormEntryId == id).ToListAsyncEF()];
entry.ComplexForms = [..await ComplexFormComponents.Where(c => c.ComponentEntryId == id).ToListAsyncEF()];
var complexFormComponents = await ComplexFormComponents.Where(c => c.ComplexFormEntryId == id || c.ComponentEntryId == id).ToListAsyncEF();
entry.Components = [..complexFormComponents.Where(c => c.ComplexFormEntryId == id)];
entry.ComplexForms = [..complexFormComponents .Where(c => c.ComponentEntryId == id)];

/// <summary>
/// Contains a definition for an entry
/// This is a CRDT object
/// </summary>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is kind of random.

@@ -1,5 +1,8 @@
namespace MiniLcm.Models;

/// <summary>
/// Contains a definition for an entry
/// </summary>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As is this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants